-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP 8.4 Support #175
base: master
Are you sure you want to change the base?
PHP 8.4 Support #175
Conversation
@paragonie-security please have a look at the failing test. I'm not certain on that one. Everything else should be good now. PHP rolled back the |
Looks weird to me. I added a test to make sure if it happens again, we can see what's happening. If you'd like, pull in https://github.com/paragonie/paseto/tree/key-length-check (temporary branch) and it should throw whenever this condition comes up |
@paragonie-security could this be merged maybe? |
throw new PasetoException('Invalid PEM format', ExceptionCode::UNSPECIFIED_CRYPTOGRAPHIC_ERROR); | ||
} | ||
|
||
$tokenizedKey = strtok($formattedKey, "\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe to do?
https://github.com/walac/glibc/blob/264aad1e6aca30efddfcfae05c44ad38bb5e137d/string/strtok.c#L25-L65
It sure seems like this would expose a timing leak that exposes the private key.
This PR includes some updates for deprecations, PHPUnit and resolved Psalm errors, as some typing and hinting has changed in PHP 8.4.
Psalm 8.4 syntax support is only supported on
dev-master
. I don't see any issues here as this is only a dev dependency.Unfortunately, at the moment, there is a newly introduced bug in PHP that's resulting in specific
DataProvider
tests failing. That should be resolved in the next minor release. See php/php-src#16870 for more details. After this is resolved, tests should be passing.